Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kernel #169

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Kernel #169

wants to merge 12 commits into from

Conversation

linneamk
Copy link
Collaborator

@linneamk linneamk commented May 12, 2021

O(N) implementation of pre-conditioned kernel calculation using implicit Fermi-Dirac response calculations.


This change is Reviewable

@nicolasbock
Copy link
Collaborator

Sorry for the delay @linneamk . Our CI tests are currently broken because we were using Travis-CI who changed their business model and stopped offering their free open source software support. We will have to switch to GitHub Actions before we can run CI again. If anyone else can verify manually that this PR is passing the tests I am happy to merge it.

@nicolasbock
Copy link
Collaborator

Could you rebase this PR? We just merged a fix for CI into master.

@nicolasbock
Copy link
Collaborator

I rebased the change.

Copy link
Collaborator

@nicolasbock nicolasbock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linneamk
Copy link
Collaborator Author

Hi! Sorry for my delay, I have updated my code and fixed the test.

@nicolasbock
Copy link
Collaborator

Thanks @linneamk . You need to rebase your changes. Right now the merge conflicts prevent us from merging the pull request.

Thanks!

@nicolasbock
Copy link
Collaborator

Thanks @linneamk .

Something went wrong during the rebase though. What commands did you run to rebase?

Copy link
Collaborator

@nicolasbock nicolasbock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need help cleaning up your rebase?

@nicolasbock
Copy link
Collaborator

Hi @linneamk

I spent a good hour just now trying to rebase your commits onto the current master branch. Unfortunately I ran into a wall and was not able to complete the rebase.

You had based the original PR on a rather old commit (4e73df7). I rebased that commit in #169 (comment) as 0d6316b.

However, you ignored that rebase and force pushed 02e93a6 which was still based on this old commit.

After asking you to rebase you merged your branch with master instead and pushed 6304e52 which is quite frankly a bit of a mess. Your PR now contains duplicate commits

* 6304e52 Fixed indentation
*   f5eeefd Rebased
|\  
| *   1fe5601 Merge branch 'master' into kernel
| |\  
| * | 02e93a6 Fixed test
| * | 2ac9f50 Added kernel routine
| * | 7960bcf O(N) pre-conditioned kernel matrix with implicit FD-expansion response
| * | 383736e Added kernel routines for XL-BOMD
* | | 1256e74 Fixed test
* | | da4eb81 Added kernel routine
* | | bf9a368 O(N) pre-conditioned kernel matrix with implicit FD-expansion response
* | | ce0e474 Added kernel routines for XL-BOMD
| |/  
|/|   
* |   eab14aa Merge pull request #201 from nicolasbock/ci_scripts

and it's unclear to me whether your changes are even correctly represented in your branch.

Merging does not always produce the desired code since the merging algorithm is really pretty dumb because it doesn't know what your intention and the intention of the person making changes in the branch you merged with was when the changes were made. It's really a best guess kind of effort.

Could you carefully go through your original changes and apply them to a fresh branch that is based on the current master branch?

Please don't hesitate to ask if any of what I wrote is unclear. Git is a little tricky to use 😄

Thanks.

@linneamk
Copy link
Collaborator Author

Hi!

Wow, I'm sorry and thank you for trying to clean up my mess 😅 I will try to create a new branch and commit my changes to, I'll
let you know if I need any help. Thanks!

@nicolasbock
Copy link
Collaborator

Thanks @linneamk ! Don't hesitate to ask if you run into any difficulties.

@jeanlucf22
Copy link
Collaborator

@linneamk Are you still working on this one? Or did these changes make it into another PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants